Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent crash on music artist grid view #1957

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

cewert
Copy link
Member

@cewert cewert commented Oct 1, 2024

m.gridTitles isn't used so I removed it.

Comes from roku.com crash log:

m                roAssociativeArray refcnt=2 count:7 
global           Interface:ifGloba$1 Local Variables: 
   file/line: pkg:/components/ItemGrid/MusicArtistGridItem.brs(17) 
#0  Function init() As Voi$1 Backtrace: 
'Dot' Operator attempted with invalid BrightScript Component or interface reference. (runtime error &hec) in pkg:/components/ItemGrid/MusicArtistGridItem.brs(17)

which points to this line after running build-prod on 2.1.4:

m.gridTitles = m.global.session.user.settings["itemgrid.gridTitles"]

Issues

Ref #1164

@cewert cewert added the bug-fix This fixes a bug. label Oct 1, 2024
@cewert cewert requested a review from a team as a code owner October 1, 2024 23:40
@neilsb
Copy link
Member

neilsb commented Oct 4, 2024

This just disables the itemText on the grids for Music Artists, right? Would we perhaps be better checking is this is valid, and if so use the setting?

With the amount we use m.global.session.user.settings perhaps a function to test it's valid and get the setting may be useful? Could even optionally take a default value to return if invalid?

The same line is also used in ItemGrid.bs

@cewert
Copy link
Member Author

cewert commented Oct 4, 2024

This just disables the itemText on the grids for Music Artists, right?

This PR shouldn't change anything since MusicArtistGridItem doesn't use m.gridTitle.

Would we perhaps be better checking is this is valid, and if so use the setting?

Definitely. I don't think this view ever worked with that setting though which is why I just focused on the crash.

With the amount we use m.global.session.user.settings perhaps a function to test it's valid and get the setting may be useful?
Could even optionally take a default value to return if invalid?

I love that idea but would rather do it in a separate PR. The old registry functions did things that way before switching to the global session. My fault for not bringing that over

The same line is also used in ItemGrid.bs

That one is different since it's setting m.top.gridTitles which is then picked up and used by GridItem

if isValid(m.itemGrid.gridTitles)

@cewert cewert merged commit a4ae801 into jellyfin:2.1.z Oct 4, 2024
13 checks passed
@cewert cewert deleted the fix-musicartistgriditem-crash branch October 4, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants